-
Notifications
You must be signed in to change notification settings - Fork 304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sync process optimizations #5219
Conversation
08243ad
to
a878147
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution!
The main ideas look good, but I don't think we can cache the bazel info
between runs. Command-line flags affect the outcome (e.g. --server_javabase), which can be in any of multiple locations and bazelrc files. Caching for a single run seems okay, since a lot of the flags that change the info output are startup flags and would invalidate most of the sync anyway.
So, if you could change the logic in BlazeInfoProvider
to invalidate every run, I think it's a great PR!
BlazeContext context, | ||
List<String> syncFlags, | ||
SyncMode syncMode) { | ||
boolean useBazelInfoRunner = !BlazeInfoProvider.isEnabled() || syncMode == SyncMode.FULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could just pass the sync mode to the BlazeInfoProvider
and have it invalidate itself (or pretend that it has invalidated) on a full sync? I quite like the idea of "everything goes through the provider".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to keep BlazeInfoProvider
useful to other use cases apart from the sync, but if we are not planning to extend it, we can make it more coupled to the sync and pass the sync mode as well.
base/src/com/google/idea/blaze/base/command/info/BlazeInfoProvider.java
Outdated
Show resolved
Hide resolved
Computing the working set involves running few git commands, including git diff, it can be skipped if the "Expand Sync to Working Set" option is disabled. The working set will be computed on each `collectProjectState` call, given that this method is triggered three times per sync operation, disabling unnecessary working set computations can reduce the sync times roughly the amount of time taken by 3 git diff runs.
a878147
to
aaad8dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! A minor nit, but otherwise thanks for the contribution!
if(!isEnabled()) { | ||
return; | ||
} | ||
BlazeInfoProvider.getInstance(project).invalidate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we check for null here? In case we de-register the provider accidentally and forget to de-register the invalidator.
aaad8dd
to
c662eb6
Compare
Currently, BlazeInfo will be computed 5 times during incremental sync, this commit introduces logic to cache it and re-use the computed value during incremental syncs. `BlazeInfoProvider` was introduced in order to avoid changing `BlazeInfoRunner` and keep its responsibility to solely execute the info command. Logic is guarded by `blaze.info.provider.enabled` which is disabled by default.
c662eb6
to
c12b09d
Compare
Tested it locally in IJ Ultimate and saw no issues. |
Checklist
Issue number: #5217
More details can be found in the issue and on each commit.